Conversation
|
|
|
@liamhuber I started looking into the possibility of including variables as well. Frankly I'm also not really convinced but I still would like to explore the possibility |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
===========================================
- Coverage 100.00% 98.81% -1.19%
===========================================
Files 30 30
Lines 1990 2022 +32
===========================================
+ Hits 1990 1998 +8
- Misses 0 24 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I just had a discussion with @XzzX and we think we can keep the current implementation is maybe not quite as useless as I had imagined, because with this implementation it is now possible to throw an error as soon as an external variable is used. |
Failing cleanly is infinitely better than looking like everything is well-provenanced when it isn't. However, I still think that restricting "ok locally defined stuff" to mean "don't use any symbols outside your function definition" is going to turn out to be a more annoying, more restrictive, more-work solution than telling people "you'll have to use well-versioned code to contribute to the KG". As an intermediate step towards catching and leveraging these external symbols, this still seems like solid progress though. |
|
@copilot Resolve the mypy issues |
Agent-Logs-Url: https://github.com/pyiron/flowrep/sessions/8c405509-6e9f-4dd2-98f3-08f895990401 Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Fixed in commit
Changes made to
|
|
@copilot Write unit tests to cover all parts introduced in this PR |
|
@copilot Write unit tests to cover all parts introduced in this PR |
…, and get_call_dependencies Agent-Logs-Url: https://github.com/pyiron/flowrep/sessions/07c0e92a-bc0a-4207-9ad4-01cc7fe4e409 Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Added in commit
Only packages available in CI (
Done in commit |
|
@copilot cover ALL lines added in this PR in the unit tests |
…verage Agent-Logs-Url: https://github.com/pyiron/flowrep/sessions/4c4a0a1c-3bb6-4d55-9da6-91a124cf181e Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Agent-Logs-Url: https://github.com/pyiron/flowrep/sessions/4c4a0a1c-3bb6-4d55-9da6-91a124cf181e Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Done in commit |
|
@liamhuber I'm gonna merge this one for now, because I think it's good enough for a first version, and as long as it is not actively used for provenance there is no imminent danger. Besides this PR is getting a bit long. |
|
Sounds good 👍 I'll look over the changes today and just open a fresh PR if there's any little patches I'm keen on. |
| def _fqns(deps: dependency_parser.CallDependencies) -> set[str]: | ||
| return {info.fully_qualified_name for info in deps} | ||
| def test_syntax_error_in_source_returns_empty_dict(self): | ||
| """When ``ast.parse`` raises ``SyntaxError``, the result must be ``{}``.""" |
There was a problem hiding this comment.
@samwaseda, can you explain this to me?
Since find_undefined_variables takes a Callable and not a source code string, shouldn't it be impossible to hit a SyntaxError because the interpreter will already have needed to parse the Callable to make it an object?
If somehow we did hit a syntax error, why would we want to let it fail gracefully? Wouldn't we want to complain to the user that we don't know what the undefined variables might be instead of simply claiming there aren't any?
There was a problem hiding this comment.
This is my only real question/concern, everything else looks great!
|
|
||
| class CallCollector(ast.NodeVisitor): | ||
| class UndefinedVariableVisitor(ast.NodeVisitor): | ||
| """AST visitor that collects used and locally-defined variable names. |
There was a problem hiding this comment.
smells like AI newline docstring 😝
Following the effort in #155, I started trying to parse variables as well. Edits will follow....
Summary
Adds undefined-variable detection to
dependency_parserto identify and resolve external symbols used within a function. This enables failing fast with a clear error when a function depends on unversioned external variables or callables, rather than silently producing incorrect provenance.Related Issue(s)
Backward Compatibility
The
get_call_dependenciesandfind_undefined_variablesfunction signatures have been narrowed: thefunc_or_varparameter type changed fromCallable | objecttoCallable[..., Any] | type[Any]. Callers passing arbitrary non-callable, non-type objects will now get a type error.Implementation Notes
UndefinedVariableVisitorAST visitor that collects used and locally-defined variable names; local (nested) function definitions raiseNotImplementedErrorto fail fast. The function name itself, all argument kinds (posonlyargs,args,kwonlyargs,*args,**kwargs), and class definitions are all tracked indefined_varsto avoid false positives.importandfrom … importstatements inside a function body are collected in.imports/.import_froms.find_undefined_variablesto extract symbols used but not defined in a function's source and resolve them viaobject_scope. Built-in names are excluded; source retrieval failures return{}gracefully; unparseable source (e.g.SyntaxError) also returns{}gracefully.get_call_dependenciesto discover dependencies from undefined variables rather than call-graph traversal; recursion into unversioned dependencies is guarded by acallable(obj) or isinstance(obj, type)type guard so non-callable values are recorded but not recursed into.from typing import Any, narrowedfunc_or_vartype annotations toCallable[..., Any] | type[Any], and added a type guard before the recursive call to satisfy static analysis.dependency_parser.py. Tests cover all new components:UndefinedVariableVisitor(function name tracking, class def tracking, import collection, top-level async def, nested local function/async function raisingNotImplementedError),find_undefined_variables(builtins excluded, graceful failure for uninspectable callables, graceful failure onSyntaxError, argument exclusion), andget_call_dependencies(empty result, cycle detection, versioned dependency collection, recursion into unversioned callables, non-callable values not recursed). Tests use only CI-available packages (pydantic,pyiron_snippets).